Pull request overview

This PR brings the Python FIDES information-flow control (integrity + confidentiality) prompt-injection/data-exfiltration defense closer to main, adding core security primitives/middleware, DevUI support for policy-violation approvals, and end-to-end samples/docs/tests.

Changes:

• Adds FIDES security samples and documentation (quick start + developer guide + implementation summary + ADR).
• Extends DevUI mapping/execution to round-trip policy-violation metadata through approval events.
• Adjusts core function-invocation plumbing and test suites to support the new approval/policy flow and keep validation green.

Reviewed changes

Copilot reviewed 14 out of 16 changed files in this pull request and generated 4 comments.

Automated Code Review

Reviewers: 1 | Confidence: 88%

✗ Design Approach

The approval-flow work mostly moves in the right direction—matching approved results by call_id is a solid improvement—but one new piece of the design is too brittle: _replace_approval_contents_with_results now detects “placeholder” tool results by searching for the magic substring [APPROVAL_PENDING] inside function_result.result. In this framework, function_result.result is just arbitrary tool output (python/packages/core/agent_framework/_types.py:809-865), so using it as control-flow state leaks middleware implementation details into the generic content model and can misfire on legitimate tool output. That should be modeled as structured state instead of string matching. I found two design-level problems in the new security layer. The feature’s isolation model depends on module-global mutable state for per-invocation context, so it will behave correctly only as long as one tool invocation runs at a time in a process. In the existing async tool pipeline, that assumption does not hold: overlapping runs can clober each other’s security context, and separately configured agents can silently overwrite each other’s quarantine client. The main design problem is in the new DevUI policy-approval path: the mapper now emits policy_violation metadata, but the existing pending-approval state and submit flow still only preserve request_id and function_call, so legitimate UI approvals will drop the metadata while _executor now tries to rebuild security-sensitive state from client input. That is both incomplete and a step away from the existing server-validated approval model. I also saw one test change that weakens coverage by turning DevTools startup failures into skips rather than failures. That means the example does not actually exercise the cross-turn security behavior it claims to demonstrate, and it teaches readers the wrong integration pattern for any policy that depends on accumulated conversation state.

Flagged Issues

• python/samples/02-agents/security/email_security_example.py expects security context to carry across two independent agent.run() calls, but core agent code creates a new session when session= is omitted (_agents.py:1185-187, 1406-1408). Scenario 2 will not run in the tainted context from scenario 1. Reuse a single AgentSession or collapse into one run.

Suggestions

• Represent pending-approval tool results with explicit metadata or a dedicated content shape, then replace them by call_id using that structured marker instead of inspecting function_result.result text.
• Keep the quarantine chat client on SecureAgentConfig/middleware state and have quarantined_llm() obtain it from the current invocation context, preserving per-agent isolation.
• Reuse a single AgentSession in the email sample's CLI scenario so the second step actually demonstrates policy enforcement after untrusted content has entered the conversation.

Automated review by eavanvalkenburg's agents

Automated Code Review

Reviewers: 4 | Confidence: 90%

✗ Correctness

The main code changes in _tools.py improve approval-flow robustness (null-checks, call_id-based matching instead of positional indexing, empty message cleanup). The observability.py fix is correct and necessary. However, there is one concrete regression: the normal success path of _auto_invoke_function silently drops additional_properties propagation from the function call to the function result, while every other code path in the same function (no-middleware direct execution, MiddlewareTermination, Exception, and error paths) continues to propagate them. This inconsistency means the same function behaves differently depending on whether middleware is present and whether execution succeds or fails. The _parse_github_mcp_labels function in security.py contains a correctness bug: it uses lexicographic string comparison (field_conf.value > most_restrictive_confidentiality.value) to determine the most restrictive confidentiality label. Because "private" < "public" in Python string ordering (since 'r' < 'u'), PRIVATE confidentiality from GitHub MCP labels is incorrectly treated as less restrictive than PUBLIC, causing PRIVATE data to remain labeled PUBLIC. This occurs at two locations (lines 764 and 782). The rest of the codebase (e.g., combine_labels, check_confidentiality_allowed) correctly uses a priority dict for this comparison.

✗ Security Reliability

The _tools.py changes improve approval-flow robustness (call_id-based matching instead of positional, null-safety on approved_function_call) and add security middleware integration points. Two reliability concerns: (1) the normal middleware success path silently drops additional_properties that error/termination paths still propagate, creating inconsistent metadata on function results; (2) placeholder replacement uses a magic string sentinel '[APPROVAL_PENDING]' in tool-result text, which is fragile—a structured Content attribute would be safer. The observability fix for non-string finish_reason is correct and needed. Two security-relevant bugs found in the new security.py module: (1) The GitHub MCP label parser uses lexicographic string comparison for confidentiality levels, causing PRIVATE content to be misclassified as PUBLIC because "private" < "public" lexicographically. This undermines confidentiality protections for GitHub MCP-sourced data. (2) Thread-local storage (threading.local()) is used to pass the middleware instance to async security tools, but threading.local() does not isolate concurrent asyncio coroutines on the same thread, risking cross-coroutine middleware leakage. The production code changes add policy-violation metadata plumbing through the approval flow. The _executor.py change spreads untrusted client-supplied dict data (**policy_violation_data) into additional_properties, which allows a malicious client to inject arbitrary keys or override the policy_violation: True flag. The _mapper.py changes safely extract specific keys. Test changes and foundry test fix are fine.

✗ Test Coverage

The PR introduces significant new logic in _tools.py — placeholder [APPROVAL_PENDING] replacement, empty-message cleanup, function_approval_request passthrough from middleware, and call_id null validation — but none of these new code paths have corresponding unit tests in test_function_invocation_logic.py. The observability isinstance(finish_reason, str) guard also lacks a test for non-string finish_reason values. While test_security.py is listed as a new file, it does not exist on disk and presumably appears in a later chunk; the gaps noted here are in the core _tools.py approval flow, which should be tested independently of the security module. The security module contains a correctness bug in _parse_github_mcp_labels where confidentiality levels are compared via string ordering ("private" < "public" alphabetically) instead of the intended hierarchy (PUBLIC < PRIVATE < USER_IDENTITY), causing PRIVATE data to not be recognized as more restrictive than PUBLIC. No test file for the security module exists on disk — test_security.py is listed as a changed file but appears in another chunk, so its coverage cannot be verified here. A test specifically covering _parse_github_mcp_labels with PRIVATE fields would catch this bug. The new test_security.py file is extensive (~2578 lines) and covers the major security subsystems well: label creation/serialization, combine_labels, ContentVariableStore, middleware label tracking, policy enforcement, auto-hiding, tiered label propagation, confidentiality checks, and quarantined LM. Three test coverage gaps stand out: (1) the devui _executor.py and _mapper.py changes add policy_violation extraction and mapping logic with zero corresponding tests, (2) test_audit_log_recording only asserts the initial empty state and never triggers a violation to verify recording, and (3) the new _replace_approval_contents_with_results tests in test_function_invocation_logic.py are well-structured and cover the key call-id-matching fix thoroughly.

✗ Design Approach

That is a contract regression rather than a local bug: the framework already treats additional_properties as the extensibility channel on Content, and the rest of _auto_invoke_function still preserves it. With middleware enabled, result metadata now disappears only on the success path, which is especially risky for any higher-level features that depend on result labels or other propagated metadata. First, it stores per-invocation middleware state in a thread-local even though the core tool loop executes function calls concurrently with asyncio.gather, so concurrent tool calls can read or clear each other’s security context. Second, the GitHub MCP label parser derives confidentiality ordering from enum string values instead of the explicit confidentiality lattice used elsewhere, which can silently downgrade private content during label propagation. I also found the new memory-test skip broad enough to hide real browser/DevTools startup regressions instead of isolating a known unsupported environment. I found two design-level problems in the new security samples. First, the email demo assumes the security context stays tainted across two separate agent.run(...) calls, but the core agent only preserves provider/history state when you reuse an explicit session, so the second step does not reliably demonstrate the policy boundary it claims to show. Second, both samples hardcode gpt-4o-mini as the quarantine Foundry model even though FoundryChatClient treats model as a deployment name, which makes the samples depend on a very specific project setup instead of a configurable quarantine deployment.

Flagged Issues

• The [APPROVAL_PENDING] placeholder replacement logic in _replace_approval_contents_with_results (lines 1872–1882 of _tools.py) is entirely new behavior that modifies message contents in-place, but has zero test coverage across all test files.

Suggestions

• Narrow the memory regression test skip — converting the generic DevTools timeout into pytest.skip() masks browser-launch and remote-debugging regressions this test is supposed to catch. Skip only for a narrowly identified unsupported-environment condition.
• Reuse an explicit session in the email sample (e.g., session = agent.create_session() passed to both agent.run() calls) so the second step genuinely demonstrates cross-turn policy enforcement.
• Make the quarantine deployment configurable (e.g., FOUNDRY_QUARANTINE_MODEL env var with fallback to FOUNDRY_MODEL) and update documentation to reference deployment names rather than a literal gpt-4o-mini string.
• Add unit tests for quarantined_llm both with and without a quarantine client configured, verifying the response is labeled UNTRUSTED and tool_choice='none' is passed.

Automated review by moonbox3's agents